Automated Test: advanced-sql-analytics #330
Conversation
… vulnerability (#94942) * disable sql expressions remove duckdb ref * Run `make update-workspace` --------- Co-authored-by: Scott Lepper <scott.lepper@gmail.com>
📝 WalkthroughWalkthroughThe pull request removes eight indirect dependencies from go.mod and introduces a local SQL abstraction layer (DB struct with stub methods) to replace external DuckDB usage. It adds a feature-flag-driven guard for SQL expressions and updates call sites to use the new in-package abstraction. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/expr/sql_command.go (1)
110-118:⚠️ Potential issue | 🟡 MinorUnreachable code after early return.
When
frame.Rows() == 0(line 110), the function setsrsp.ValuestoNoData(lines 111-113) but does not return. Execution continues to lines 116-118, which overwritesrsp.ValueswithTableData. This means theNoDatabranch has no effect.Proposed fix: add early return
if frame.Rows() == 0 { rsp.Values = mathexp.Values{ mathexp.NoData{Frame: frame}, } + return rsp, nil } rsp.Values = mathexp.Values{ mathexp.TableData{Frame: frame}, }
🤖 Fix all issues with AI agents
In `@pkg/expr/reader.go`:
- Around line 194-200: The enableSqlExpressions function currently inverts the
feature flag and returns false in all branches, so change its implementation in
the enableSqlExpressions function to a single clear return (make it return false
unconditionally as a stub), removing the inverted call to
h.features.IsEnabledGlobally; keep the function signature
(enableSqlExpressions(*ExpressionQueryReader)) and add a TODO comment
referencing SQL implementation when re-enabling.
In `@pkg/expr/sql/db.go`:
- Around line 12-14: Remove the unused method DB.TablesList which returns a "not
implemented" error: delete the DB.TablesList receiver method from
pkg/expr/sql/db.go so the type DB no longer exposes TablesList; leave the
existing package-level TablesList function (in parser.go) as the public API that
callers (e.g., code referencing TablesList in sql_command.go and tests) continue
to use and ensure no other code references the removed method.
🧹 Nitpick comments (1)
pkg/expr/sql/db.go (1)
9-26: Add documentation for stub implementations.This file introduces a stub
DBtype that replaces the external DuckDB dependency. All methods return"not implemented"errors. Adding documentation would help future maintainers understand the design intent.Proposed documentation
+// DB provides an in-memory SQL execution engine. +// TODO: This is currently a stub implementation. Methods will be implemented +// when the SQL expressions feature is ready for release. type DB struct { } +// TablesList extracts table names referenced in a SQL statement. func (db *DB) TablesList(rawSQL string) ([]string, error) { return nil, errors.New("not implemented") } +// RunCommands executes a list of SQL commands and returns the result. func (db *DB) RunCommands(commands []string) (string, error) { return "", errors.New("not implemented") } +// QueryFramesInto executes a query against the provided data frames +// and populates the result into the target frame. func (db *DB) QueryFramesInto(name string, query string, frames []*data.Frame, f *data.Frame) error { return errors.New("not implemented") } +// NewInMemoryDB creates a new in-memory database instance. func NewInMemoryDB() *DB { return &DB{} }
| func enableSqlExpressions(h *ExpressionQueryReader) bool { | ||
| enabled := !h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions) | ||
| if enabled { | ||
| return false | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
Critical: Function always returns false due to logic errors.
The enableSqlExpressions function has two issues that cause it to always return false:
- Inverted condition (line 195):
enabled := !h.features.IsEnabledGlobally(...)meansenabledistruewhen the flag is OFF. - Both branches return false (lines 196-199): Regardless of
enabled, the function returnsfalse.
This renders the feature flag ineffective — SQL expressions are permanently disabled.
Proposed fix
func enableSqlExpressions(h *ExpressionQueryReader) bool {
- enabled := !h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions)
- if enabled {
- return false
- }
- return false
+ return h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions)
}If the intent is to always disable SQL expressions (since the implementation uses stubs), simplify to:
func enableSqlExpressions(_ *ExpressionQueryReader) bool {
return false // TODO: Enable when SQL implementation is complete
}🤖 Prompt for AI Agents
In `@pkg/expr/reader.go` around lines 194 - 200, The enableSqlExpressions function
currently inverts the feature flag and returns false in all branches, so change
its implementation in the enableSqlExpressions function to a single clear return
(make it return false unconditionally as a stub), removing the inverted call to
h.features.IsEnabledGlobally; keep the function signature
(enableSqlExpressions(*ExpressionQueryReader)) and add a TODO comment
referencing SQL implementation when re-enabling.
| func (db *DB) TablesList(rawSQL string) ([]string, error) { | ||
| return nil, errors.New("not implemented") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if DB.TablesList is used anywhere
echo "=== Searching for DB.TablesList usage ==="
rg -n "\.TablesList\(" --type go
echo ""
echo "=== Searching for TablesList calls (broader) ==="
rg -n "TablesList" --type go | head -50
echo ""
echo "=== Checking parser.go implementation ==="
if [ -f "pkg/expr/sql/parser.go" ]; then
wc -l pkg/expr/sql/parser.go
head -100 pkg/expr/sql/parser.go
fiRepository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 3876
Remove unused DB.TablesList method.
The DB.TablesList method is never called in the codebase. The package-level TablesList function in parser.go is the actual public API and is used throughout (e.g., sql_command.go:30, extensively in tests). Since DB.TablesList returns "not implemented" and the package-level function doesn't delegate to it, the method serves no purpose and should be removed to avoid API confusion.
🤖 Prompt for AI Agents
In `@pkg/expr/sql/db.go` around lines 12 - 14, Remove the unused method
DB.TablesList which returns a "not implemented" error: delete the DB.TablesList
receiver method from pkg/expr/sql/db.go so the type DB no longer exposes
TablesList; leave the existing package-level TablesList function (in parser.go)
as the public API that callers (e.g., code referencing TablesList in
sql_command.go and tests) continue to use and ensure no other code references
the removed method.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
Chores
Refactor